- 
          
- 
                Notifications
    You must be signed in to change notification settings 
- Fork 60
feat: Enable fuzzy search for collection auto-add #2871
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
f6dfaa3    to
    a6ed3e3      
    Compare
  
    a6ed3e3    to
    20d3c97      
    Compare
  
    There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Works brilliantly, great improvement!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall this is an improvement for sure, but there's a few things I'm a little confused about and a couple of bugs/unexpected behaviours I was able to find.
| private readonly searchTask = new Task(this, { | ||
| task: async ([names], { signal }) => { | ||
| if (!names || signal.aborted) { | ||
| return; | ||
| } | ||
|  | ||
| return new Fuse(names, { threshold: 0.4, minMatchCharLength: 2 }); | ||
| }, | ||
| args: () => [this.searchValuesTask.value] as const, | ||
| }); | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure I understand why this is a Task — it seems to me that this could go in a willUpdate, because all of the code here is synchronous. As such, the pending state on line 238 is never rendered, since the task always immediately successfully completes whenever it's run.
| return; | ||
| } | ||
|  | ||
| return new Fuse(names, { threshold: 0.4, minMatchCharLength: 2 }); | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should minMatchCharLength be MIN_SEARCH_LENGTH? As is, when starting to search for something, you don't get any results until you've typed more than two characters, even though when you've only typed one character you get both a) results back from the search prefix endpoint, if there's any collections starting with the character you've entered, and b) a message that says "no matching collections found", which isn't true.
| private readonly searchResultsTask = new Task(this, { | ||
| task: async ([searchByValue, hasSearchStr], { signal }) => { | ||
| if (!hasSearchStr) return []; | ||
| const data = await this.fetchCollectionsByPrefix(searchByValue, signal); | ||
| let searchResults: Collection[] = []; | ||
| if (data?.items.length) { | ||
| searchResults = this.filterOutSelectedCollections(data.items); | ||
| } | ||
| return searchResults; | ||
| }, | ||
| args: () => [this.searchByValue, this.hasSearchStr] as const, | ||
| }); | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Correct me if I'm wrong, but I don't think the results of this are used anywhere, are they?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, it looks like currently onSearchInput serves to re-render the component, and nothing else, so it could functionally be replaced with this.requestUpdate()
Resolves #2846
Changes
Manual testing
Screenshots